fix(components/toast): add new token value for last toast when display direction is NewestOnTop#4382
fix(components/toast): add new token value for last toast when display direction is NewestOnTop#4382Blackbaud-SandhyaRajasabeson wants to merge 2 commits into13.x.xfrom
NewestOnTop#4382Conversation
📝 WalkthroughWalkthroughThe changes implement a CSS class-based mechanism for the "newest on top" toast display direction. A protected signal in the toaster component conditionally applies the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
View your CI Pipeline Execution ↗ for commit aaf9b0b
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/components/toast/src/lib/modules/toast/toast.component.scss (1)
259-263: Use an override variable for the new bottom margin token.This new rule directly uses
--sky-comp-omnibar-toaster-space-inset-bottom; it should route throughsky-default-overrideswith a fallback, consistent with the file’s override pattern.As per coding guidelines, "When using a CSS variable with its current value, find the existing location matching the variable name, assign the new variable that value, and use the new variable in the current location with the given fallback."Proposed fix
`@include` compatMixins.sky-default-overrides('.sky-toast') { @@ --sky-override-toast-info-success-exclamation-path-color: #{$sky-highlight-color-info}; --sky-override-toast-margin-bottom: #{$sky-margin-double}; + --sky-override-toast-margin-bottom-newest-on-top-last-child: var(--sky-comp-omnibar-toaster-space-inset-bottom); --sky-override-toast-padding: 0 #{$sky-padding}; } @@ .sky-toaster-newest-on-top { sky-toast:last-child { .sky-toast { - margin-bottom: var(--sky-comp-omnibar-toaster-space-inset-bottom); + margin-bottom: var( + --sky-override-toast-margin-bottom-newest-on-top-last-child, + var(--sky-comp-omnibar-toaster-space-inset-bottom) + ); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/toast/src/lib/modules/toast/toast.component.scss` around lines 259 - 263, The rule in .sky-toaster-newest-on-top > sky-toast:last-child > .sky-toast uses --sky-comp-omnibar-toaster-space-inset-bottom directly; instead add a corresponding override variable in the sky-default-overrides block (e.g., --sky-toast-omnibar-space-inset-bottom) that falls back to --sky-comp-omnibar-toaster-space-inset-bottom, and then use that new override variable in the .sky-toast margin-bottom declaration so it follows the file's override pattern and provides a proper fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/components/toast/src/lib/modules/toast/toast.component.scss`:
- Around line 259-263: The rule in .sky-toaster-newest-on-top >
sky-toast:last-child > .sky-toast uses
--sky-comp-omnibar-toaster-space-inset-bottom directly; instead add a
corresponding override variable in the sky-default-overrides block (e.g.,
--sky-toast-omnibar-space-inset-bottom) that falls back to
--sky-comp-omnibar-toaster-space-inset-bottom, and then use that new override
variable in the .sky-toast margin-bottom declaration so it follows the file's
override pattern and provides a proper fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f2eba9b-111a-41fc-b63e-265e4cb5d9f1
⛔ Files ignored due to path filters (1)
apps/code-examples/src/assets/stack-blitz/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
libs/components/toast/src/lib/modules/toast/toast.component.scsslibs/components/toast/src/lib/modules/toast/toaster.component.spec.tslibs/components/toast/src/lib/modules/toast/toaster.component.ts
|
Component Storybooks: Apps: |
Blackbaud-SteveBrush
left a comment
There was a problem hiding this comment.
Do you mind adding a toggle to the playground to set newestOnTop so we can test it out manually? Also, if a visual test is valuable, we should consider writing one.
|
|
||
| protected isNewestOnTop = signal( | ||
| this.#containerOptions?.displayDirection === | ||
| SkyToastDisplayDirection.NewestOnTop, |
There was a problem hiding this comment.
Not to whoever picks up this story next: This needs to be a computed signal so that the spacing changes if consumers change direction
AB#3652195
Summary by CodeRabbit